There are a lot of package download failures in the failing Azure build.

@SteveGilham Thanks for this. Interesting approach. However, I have some minor issues with it:

• Dotnet needs to be in PATH while the Fake.DotNet.Cli has all the logic to download and install a particular SDK version on-demand. Whatever we decide to do it should be compatible with such a local installation without the need to set any environment variables manually.
• For local tools, I think we should "detect" if dotnet tool restore is required and call it automatically when using whatever API we decide to create

I'm not sure how to fix the issues. From a "low" level API perspective we could easily create a helper similar to CreateProcess.withFramework for local and global tools.
These helpers might need to be part of Fake.DotNet.Cli to implement the logic from above (and even install the SDK on demand).

From an "high" level API perspective (for example Fake.Testing.ReportGenerator in this case) our options are quite limited indeed:

• We could either add a new field as you proposed here. This has the "downside" that for every such change we need to update ALL modules.
• We could push people to use the CreateProcess APIs and return a CreateProcess instance. This has the unfortunate side-effect that calling external stuff is more verbose.

Some other questions are:

• Should we install local/global tools automatically if they don't exist?

What do you think?

This proposal is the "simplest thing that could possibly work" for this period of transition from the old .net Framework world to the new .net core one -- with the tool helper types all still in the old world where just an ExePath parameter with no further context made sense for .net-based tools (native tools shouldn't be affected).

The goals were

• no breaking API changes (keep .ExePath for the moment, but allow it to be deprecated)
• specify which flavour of the tool (by name, by dotnet name, by path)
• no extra coupling (avoiding linking fake.dotnet.cli.dll)
• no extra behaviour -- still just assemble a command line from a more strongly typed structure and launch it, and no more
• use one tool-helper as a prototype (ReportGenerator chosen purely as an immediate pain point of my own)

I dismissed the idea of explicitly injecting just a process runner function (opaquely take command and arguments, and do the launching), as that wouldn't tell the tool helper whether it should do a withFramework, or maybe add a .exe to the name; as well as there being issues with injecting a curried DotNet.exec and the resulting multiple places values like the working directory could be set. With further cases assembled, some common process runner code could be factored out, either into ProcessUtils, or a ToolHelperUtils with all the new types and functions, but that would be all implicit and selected by the tool helper based on the specified type of tool.

In the existing ReportGenerator helper, there's not even a test to see if the ReportGenerator.exe exists, let alone a fallback download of the tool as a NuGet package, so an equivalent on the .net core side just for local tools (but not global or original dotnet CLI ones) doesn't seem like it belongs. The proposal preserves the existing behaviour by expecting the tool to be present and runnable, whatever its origin.

A DotNet.tool setParams API could be a further useful helper, but that's a separate job of work. There's also a separate piece of work adding the new option of Fake as local tool to the bootstrapping page.

In the case where the desired dotnet.exe version is on the build machine, but not on the PATH where it can be invoked simply as dotnet, then the DotNetTool type provides a way to specify where it is -- the same location would have to be on hand to set the DotNetCliPath if using DotNet.exec anyway.

Unfortunately, I don't understand some parts of your response. Please let me know if I got something wrong. Regarding your goals, I generally agree, but I think a dependency on Fake.DotNet.Cli might be ok considering that's basically what local and global tools depend on as well.

I dismissed the idea of explicitly injecting just a process runner function (opaquely take command and arguments, and do the launching), as that wouldn't tell the tool helper whether it should do a withFramework, or maybe add a .exe to the name;

I think only the Tool helper knows how the process has to be started (ie if withFramework is required or not?)

as well as there being issues with injecting a curried DotNet.exec and the resulting multiple places values like the working directory could be set.

??

With further cases assembled, some common process runner code could be factored out, either into ProcessUtils, or a ToolHelperUtils with all the new types and functions, but that would be all implicit and selected by the tool helper based on the specified type of tool.

As far as I understand this paragraph this matches my suggestion with creating a functionality similar to withFramework for helpers to be used?

A DotNet.tool setParams API could be a further useful helper, but that's a separate job of work. There's also a separate piece of work adding the new option of Fake as local tool to the bootstrapping page.

Indeed we could track the docs in a separate issue but with global and local tools we should have some idea how the API should look like, because I don't want to introduce breaking changes later or different looking and behaving APIs across all helpers...

then the DotNetTool type provides a way to specify where it is

Yes but my point is that fake should handle that internally when you use the DotNet.install API as it is not straightforward to get the path to the dotnet exe. We could add documentation (but we need that on every helper page) but I'd prefer a different API.

Yes, the ideal would be to have a CreateProcess.withDotNet function, something akin to

        let withDotNet (buildOptions: DotNet.Options -> DotNet.Options) (c:CreateProcess<_>) =
// magic happens here

in form, with the body calling into code shared with (newly factored out of) DotNet.exec, but published from Fake.DotNet.Cli

Consuming code in the tool helper would look like

    CreateProcess.fromCommand (RawCommand(tool, args))
    |> match parameters.ToolType with
       | Fake.Core.ProcessUtils.ToolType.Framework _ -> CreateProcess.withFramework
       | Fake.Core.ProcessUtils.ToolType.DotNet t -> CreateProcess.withDotNet (buildOptions t)
       | _ -> id
    |> CreateProcess.withWorkingDirectory parameters.WorkingDir
   ...

With the current proposed API, the buildOptions function would look like

   let buildOptions t o = { o with WorkingDirectory = parameters.WorkingDir
                                               DotNetCliPath = match t.DotNetCli with
                                               | Some path = path
                                               | _ -> o.DotNetCliPath }

If instead the API allowed a user supplied DotNet.Options -> DotNet.Options function rather than just the CLI path, the invoking line would be

       | Fake.Core.ProcessUtils.ToolType.DotNet t -> CreateProcess.withDotNet buildOptions

where

   let buildOptions = userSuppliedFunction >> (fun o -> { o with WorkingDirectory = parameters.WorkingDir }

simply to make the working directory consistent throughout.

The bit that's still TBD as far as I can see would be handling the actual launch -- the withGlobalJson and output redirection behaviour of DotNet.exec -- in the more CreateProcess API model. I shall have to think on this in the next few days.

I think it needs to be

        let withDotNetTool (packageName:string) (exeName:string) (buildOptions: DotNet.Options -> DotNet.Options) (c:CreateProcess<_>) =
// magic happens here

Also considering this issue the signature of DotNet.install might have been a mistake, returning record which includes the path to the exe might have been a better idea. This way we wouldn't have DotNet.Options -> DotNet.Options in withDotNetTool which might be more confusing (as most parameters will be ignored). We could obsolete it in favor of a new API.

Additionally, given that most tools will have the match deciding on withFramework and withDotNetTool we probably should have a helper for it as well:

        let witFrameworkOrDotNetTool (type:ToolType) (packageName:string) (exeName:string) (buildOptions: DotNet.Options -> DotNet.Options) (c:CreateProcess<_>) =
// magic happens here

Please merge release/next to get a green build

In the API I've used tool name rather than package name because they often aren't the same e.g.

Package Id                 Version      Commands                                                                        ------------------------------------------------------
fantomas-tool              3.0.0        fantomas
project2015to2017.cli      1.8.1        csproj-to-2017
sourcelink                 3.0.0        sourcelink

I have a commit on top of this that applies the change to Fake.DotNet.Paket, for some reason I can't PR onto your branch @SteveGilham but here it is: https://github.com/Tarmil/FAKE/tree/issue-2398

@Tarmil I've merged the change into this PR for you.

Not sure what's up with the Azure build; it looks like it managed to lose the ProjectReference dependency that the other CI servers were happy with.

The Azure Build is the only one building the legacy fake 4 package which is a separate pre sdk project file.

To clarify: For the old package it is only important to stay "releasable", there is no need to port the features. But most likely just adding the new files is easier to add the required #if-defs.

We seem to have crossed in the middle here (I didn't check for updates to the conversation before committing the changes).

In the one remaining contentious bit, the usage I see is like this

    let tool = parameters.ToolType.Command parameters.ExePath "reportgenerator"

where the tool helper supplies some defaults -- a path for framework implementations and a "most likely" name for .net core tools, and allows the user to override. In most cases, "most likely" will be the one name used both for global and CLI tool variants -- awkward cases where the names differ will just have to document what the particular choice of "most likely" is.

See SteveGilham#1

I tried several changes (I don't think they are runnable just yet, but ready for some discussions:

• Make CreateProcess the base type for everything, this makes downstream processing more easy as you should always be able to use Proc.run no matter the actual configuration (this removes your suggestion to introduce a new type and new runner functions in Proc)
• Previously you used

  let tool = parameters.ToolType.Command parameters.ExePath "reportgenerator"
  CreateProcess.fromCommand (RawCommand(tool, args))

  I think this "reportgenerator" is currently missing. I'm not sure if we should add it as an argument or extend your suggested types.
• I think I will add more points later.

Working on this was good to get an idea on why you choose some routes. I guess I need a break and will continue later today or tomorrow as there are still some open issues in my proposal.

As this change is becoming a significant re-architecting, especially compared with the initial minimal impact proposal, I'll step back for the moment, and just merge updates to the PR as they arrive.

As this change is becoming a significant re-architecting

I hope it is not, the idea is to adapt the current architecture to support this use-case (which it should).

I'll step back for the moment, and just merge updates to the PR as they arrive.

But I understand your concerns, just thought it would good to have a second set of eyes to take a look (for example if all use-cases are still possible and the APIs are reasonable). In this case I'll just push to your branch directly, if you have left the corresponsing checkbox active :)

I've not explicitly checked or unchecked anything, but I'll look out for messages just in case.

The current implementation should now work.

It can be combined with an existing DotNet.install call like this:

let install = lazy DotNet.install // 

let cp =
    rawCreateProcess (fun p ->
       { p with
             ToolType = ToolType.CreateLocalTool(install.Value) })

We still need to go through all tools and add the parameter (ie replace CreateProcess.withFramework with the new CreateProcess.withToolType), but that can be a community effort (I added some docs).

Now we should take a close look at all the new APIs and check if the naming is OK, which I doubt (and if we should add docs).
Conceptually it looks OKish to me. Some tests probably need to be fixed.

The remaining references to CreateProcess.withFramework in the current code-base are in

• Fsc.fs
• NuGet.fs
• NUnit3.fs
• SpecFlowNext.fs
• SonarQube.fs
• Pickles.fs

A brief search hasn't turned up netstandard based versions of any of these tools aside from the .net core SDK 3.0 version of fsc.exe as a self-contained executable; paket and ReportGenerator have been on the bleeding edge by comparison.

Thanks for taking a look. I think I'll also update the installation docs to recommend the new local tool installation option (maybe even as preferred variant?). Which basically completes "initial dotnet sdk local tool support" from a fake standpoint

There's nothing in the API naming that looks obviously wrong or out of place to me in the ToolType-related APIs; I've not looked deeply into the re-workings of the CreateProcess internals, for any new public surface.